Skip to content

gh-139748: fix leaks in AC error paths when using FS-based converters#139765

Merged
kumaraditya303 merged 5 commits into
python:mainfrom
picnixz:fix/leak/compile-139748
Oct 8, 2025
Merged

gh-139748: fix leaks in AC error paths when using FS-based converters#139765
kumaraditya303 merged 5 commits into
python:mainfrom
picnixz:fix/leak/compile-139748

Conversation

@picnixz

@picnixz picnixz commented Oct 8, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-app bedevere-app Bot mentioned this pull request Oct 8, 2025
Closed
@picnixz
picnixz force-pushed the fix/leak/compile-139748 branch 2 times, most recently from 3478a0a to 42bead3 Compare October 8, 2025 12:07
@AA-Turner

AA-Turner commented Oct 8, 2025

Copy link
Copy Markdown
Member

Why weren't/aren't these refleaks caught by the refleak buildbots?

A

@picnixz picnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Oct 8, 2025
@picnixz

picnixz commented Oct 8, 2025

Copy link
Copy Markdown
Member Author

Why weren't/aren't these refleaks caught by the refleak buildbots?

  1. Because those leaks only occur on error paths which are usually not tested.
  2. Even if error paths are tested, if the string is actually immortal, it won't be a leak. You won't see the leak if you use compile(b'', '?', 'exec', optimize=99999999999) because ? is immortal.
  3. For successful paths, there is code inside the implementation that does a manual Py_DECREF on something that is meant to be a borrowed reference!

@picnixz
picnixz force-pushed the fix/leak/compile-139748 branch 3 times, most recently from a24bf3b to 9f89608 Compare October 8, 2025 13:43
@picnixz
picnixz force-pushed the fix/leak/compile-139748 branch from 9f89608 to bf3d10d Compare October 8, 2025 13:56
vstinner
vstinner approved these changes Oct 8, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kumaraditya303
kumaraditya303 merged commit b04a57d into python:main Oct 8, 2025
59 checks passed
@miss-islington-app

miss-islington-app Bot commented Oct 8, 2025

Copy link
Copy Markdown

Thanks @picnixz for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app

miss-islington-app Bot commented Oct 8, 2025

Copy link
Copy Markdown

Sorry, @picnixz and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b04a57deef66ce08233be57d1ab5873388df2cea 3.14

@miss-islington-app

miss-islington-app Bot commented Oct 8, 2025

Copy link
Copy Markdown

Sorry, @picnixz and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b04a57deef66ce08233be57d1ab5873388df2cea 3.13

kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
59a091b 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 8, 2025
Merged
@bedevere-app

bedevere-app Bot commented Oct 8, 2025

Copy link
Copy Markdown

GH-139789 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Oct 8, 2025
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
1b08792 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
50efd4b 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 8, 2025
Merged
@bedevere-app

bedevere-app Bot commented Oct 8, 2025

Copy link
Copy Markdown

GH-139792 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Oct 8, 2025
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.13] pythongh-139748: fix leaks in AC error paths when using unicod…
786b335 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 added a commit that referenced this pull request Oct 8, 2025
@kumaraditya303 @picnixz
[3.14] gh-139748: fix leaks in AC error paths when using unicode FS-b… (
90cd009 
#139789)

* [3.14] gh-139748: fix leaks in AC error paths when using unicode FS-based converters (GH-139765)
(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 added a commit that referenced this pull request Oct 8, 2025
@kumaraditya303 @picnixz
[3.13] gh-139748: fix leaks in AC error paths when using unicode FS-b… (
dac827c 
#139792)

* [3.13] gh-139748: fix leaks in AC error paths when using unicode FS-based converters (GH-139765)
(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz
picnixz deleted the fix/leak/compile-139748 branch October 11, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Assignees

@kumaraditya303 kumaraditya303

Labels

topic-argument-clinic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@picnixz @AA-Turner @vstinner @kumaraditya303